Repository navigation
Support per-subject experimentation opt-out - #12950
leoromanovsky wants to merge 14 commits into
Conversation
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
…imentation-opt-out-immediate # Conflicts: # metadata/supported-configurations.json # products/feature-flagging/feature-flagging-api/src/main/java/datadog/trace/api/openfeature/Provider.java
…imentation-opt-out-immediate
…imentation-opt-out-immediate
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
More details
Both providers preserve ordinary fallback delivery when experiment allocations are excluded, and malformed activation metadata is isolated to the affected flag. Validation was limited to static review.
🤖 Bits Code Review · Commit 660b5b9
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660b5b9611
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| final Instant experimentationCutoff; | ||
| try { | ||
| experimentationCutoff = ExperimentationOptOut.cutoff(experimentationOptOutMode, context); |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Should an invalid dd_experimentation_opt_out_at skip only the doLog=true allocations, instead of returning the default for every flag?
With an enabled mode, one malformed or non-string value returns INVALID_CONTEXT here, before the flag lookup. Every flag for that subject then returns its code default. That includes kill switches, plain rollouts, and flags with no experiment allocation. This follows draft decisions D002 and D016 in DataDog/ffe-system-test-data#55, and libdatadog does the same. But it removes ordinary feature delivery, which opt-out is meant to keep.
Both designs keep experiment allocations closed. They differ for ordinary allocations and for flags that do not exist.
Inputs that reach this path — each one returns the default for every flag
Executed against this PR's FlagEvaluator, with an enabled mode:
| Context value | Result on a flag with no doLog=true allocation |
|---|---|
java.time.Instant, through MutableContext.add(key, Instant) |
INVALID_CONTEXT, default |
"2026-10-19" (date only) |
INVALID_CONTEXT, default |
a JSON-null, structure, or list Value |
INVALID_CONTEXT, default |
| any of the above, on a flag that does not exist | INVALID_CONTEXT, not FLAG_NOT_FOUND |
Instant is a first-class OpenFeature Java value, so a Java integration can pass it without noticing the problem.
Options — I prefer 1
- Change the contract. An invalid cutoff skips every
doLog=trueallocation and evaluates the rest as usual. Experiments still fail closed, and a bad integration keeps ordinary delivery. libdatadog and the other SDKs would need the same change. - Keep D016, and make the scope explicit. The README already says invalid values return
INVALID_CONTEXT. Add: "An invalid value makes every flag evaluation for that context return the application default withINVALID_CONTEXT. This includes flags without experiments and flags that do not exist. Pass the timestamp as an RFC 3339String.java.time.Instantvalues are rejected."
A sketch of option 1. I did not compile it, because it depends on the contract decision:
// ExperimentationOptOut: cutoff() returns this sentinel where it throws today.
static final Instant INVALID_CUTOFF = Instant.MIN;
// ExperimentationOptOut.excludes: an invalid cutoff excludes every exposure-logging allocation.
if (cutoff == INVALID_CUTOFF) {
return Boolean.TRUE.equals(allocation.doLog);
}The future-mode activation check in FlagEvaluator would also need to skip an invalid cutoff, so a malformed randomizationActivatedAt does not fail the flag for that subject.
| if (value != null && value.isString()) { | ||
| try { | ||
| // Use the same timestamp parser as allocation startAt/endAt. | ||
| return DateTimeFormatter.ISO_OFFSET_DATE_TIME.parse(value.asString(), Instant::from); |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Replace ISO_OFFSET_DATE_TIME with a parser for the shared timestamp grammar, and test it against the shared corpus. Suggest matching the iOS and Android opt-out PRs, which use a hand-written parser.
ISO_OFFSET_DATE_TIME is not the grammar in draft decisions D005–D007. On the 50-row shared timestamp corpus, Java disagrees on 8 rows. libdatadog and the iOS, Android, and browser opt-out parsers agree on all 50. So one stored cutoff can opt a subject out in a Java service and return INVALID_CONTEXT on every flag in a Ruby service. randomizationActivatedAt uses the same formatter (UniversalFlagConfigParser.java:239), so the future-mode PARSE_ERROR rule also fires on different inputs.
timestampValidationMatchesAllocationTimestamps cannot catch this, because it takes its expected result from ISO_OFFSET_DATE_TIME itself.
1. The 8 corpus rows where Java disagrees — JDK 21, this PR's cutoff(); ids without the timestamp/ prefix
| Corpus id | Input | Contract | Java |
|---|---|---|---|
ten-digits-zero |
2026-10-19T12:00:00.1234567890Z |
accept, truncate to 9 digits | reject |
ten-digits-nine |
2026-10-19T12:00:00.1234567899Z |
accept | reject |
twelve-digits |
2026-10-19T12:00:00.123456789999Z |
accept | reject |
fraction-zero |
2026-01-01T00:00:00.000000000000Z |
accept | reject |
offset-max |
2026-01-01T00:00:00+23:59 |
accept | reject (java.time caps offsets at ±18:00) |
utc-year-underflow |
0000-01-01T00:00:00+01:00 |
reject | accept |
utc-year-overflow |
9999-12-31T23:59:59-01:00 |
reject | accept |
empty-fraction |
2026-01-01T00:00:00.Z |
reject | accept |
Outside the corpus, Java also accepts 2026-10-19T12:00Z (no seconds), +01:00:30 (seconds in the offset), +01 (hour-only offset), and +10000-01-01T00:00:00Z. The contract rejects all four. The first is what OffsetDateTime.toString() prints when the seconds and nanoseconds are zero.
2. The other opt-out parsers I checked — the same 50 rows, run through a replica of each parser
| SDK | Parser | Rows that disagree |
|---|---|---|
| libdatadog (Ruby, Python, PHP) | hand-written | 0 |
| iOS, DataDog/dd-sdk-ios#3338 | hand-written regex and calendar checks | 0 |
| Android, DataDog/dd-sdk-android#4024 | regex, then non-lenient SimpleDateFormat |
0 (desktop JDK replica; not run on an Android runtime) |
| browser, DataDog/openfeature-js-client#423 | hand-written | 0 |
| Java, this PR | ISO_OFFSET_DATE_TIME |
8 |
| Go, DataDog/dd-trace-go#5666 (merged) | time.Parse(time.RFC3339Nano, …) |
10 |
Go rejects lowercase t/z. It accepts a comma fraction, +24:00, +01:60, a one-digit hour, and out-of-range UTC years. That is outside this PR, but it is the same class of gap. Apart from libdatadog's own suite, none of these SDKs' tests run the corpus.
3. Suggested fix — a port of libdatadog's parser, wired into both call sites
package com.datadog.openfeature.internal.ufc;
import java.time.DateTimeException;
import java.time.Instant;
import java.time.LocalDate;
import java.time.ZoneOffset;
import javax.annotation.Nullable;
/**
* Contract timestamp grammar (D005-D007) shared by the subject cutoff and allocation activation.
*/
public final class OptOutTimestamps {
private OptOutTimestamps() {}
/** Returns the instant, or null when {@code s} is outside the contract grammar. */
@Nullable
public static Instant parse(final String s) {
final int n = s.length();
if (n < 20
|| s.charAt(4) != '-'
|| s.charAt(7) != '-'
|| (s.charAt(10) != 'T' && s.charAt(10) != 't')
|| s.charAt(13) != ':'
|| s.charAt(16) != ':') {
return null;
}
final int year = digits(s, 0, 4);
final int month = digits(s, 5, 7);
final int day = digits(s, 8, 10);
final int hour = digits(s, 11, 13);
final int minute = digits(s, 14, 16);
final int second = digits(s, 17, 19);
if (year < 0
|| month < 0
|| day < 0
|| hour < 0
|| hour > 23
|| minute < 0
|| minute > 59
|| second < 0
|| second > 59) {
return null;
}
int i = 19;
int nanos = 0;
if (s.charAt(i) == '.') {
final int start = ++i;
int scale = 100_000_000;
while (i < n && s.charAt(i) >= '0' && s.charAt(i) <= '9') {
nanos += (s.charAt(i) - '0') * scale; // truncate beyond 9 digits
scale /= 10;
i++;
}
if (i == start) {
return null;
}
}
final int offsetSeconds;
final int rest = n - i;
if (rest == 1 && (s.charAt(i) == 'Z' || s.charAt(i) == 'z')) {
offsetSeconds = 0;
} else if (rest == 6 && (s.charAt(i) == '+' || s.charAt(i) == '-') && s.charAt(i + 3) == ':') {
final int oh = digits(s, i + 1, i + 3);
final int om = digits(s, i + 4, i + 6);
if (oh < 0 || oh > 23 || om < 0 || om > 59) {
return null;
}
offsetSeconds = (s.charAt(i) == '-' ? -1 : 1) * (oh * 3600 + om * 60);
} else {
return null;
}
final Instant instant;
try {
instant =
LocalDate.of(year, month, day)
.atTime(hour, minute, second, nanos)
.toInstant(ZoneOffset.UTC)
.minusSeconds(offsetSeconds);
} catch (final DateTimeException e) {
return null;
}
final int utcYear = instant.atOffset(ZoneOffset.UTC).getYear();
return utcYear < 0 || utcYear > 9999 ? null : instant;
}
private static int digits(final String s, final int from, final int to) {
int v = 0;
for (int i = from; i < to; i++) {
final char c = s.charAt(i);
if (c < '0' || c > '9') {
return -1;
}
v = v * 10 + (c - '0');
}
return v;
}
}--- a/dd-openfeature/src/main/java/com/datadog/openfeature/internal/ufc/UniversalFlagConfigParser.java
+++ b/dd-openfeature/src/main/java/com/datadog/openfeature/internal/ufc/UniversalFlagConfigParser.java
@@ -236,7 +236,8 @@ public final class UniversalFlagConfigParser {
}
final Map<?, ?> json = asObject(value, "allocation");
final Object activation = json.get("randomizationActivatedAt");
- final Instant parsedActivation = activation instanceof String ? toInstant(activation) : null;
+ final Instant parsedActivation =
+ activation instanceof String ? OptOutTimestamps.parse((String) activation) : null;
return new Allocation(
asString(json.get("key"), "key"),
toList(json.get("rules"), "rules", UniversalFlagConfigParser::toRule),In ExperimentationOptOut.cutoff, with import com.datadog.openfeature.internal.ufc.OptOutTimestamps;:
if (value != null && value.isString()) {
final Instant cutoff = OptOutTimestamps.parse(value.asString());
if (cutoff != null) {
return cutoff;
}
}
throw new IllegalArgumentException(INVALID_TIMESTAMP);startAt and endAt keep their current parser.
4. Corpus-driven test — replaces timestampValidationMatchesAllocationTimestamps
I copied the corpus into src/test/resources/experimentation-opt-out-timestamps.jsonl from libdatadog's libdd-ffe-test-suite/tests/data/experimentation-opt-out-timestamps.jsonl. The same 50 rows are the "kind": "timestamp" lines of contracts/experimentation-opt-out/corpus/browser-proposal.jsonl in ffe-system-test-data. Once DataDog/ffe-system-test-data#55 merges, the test can read them from the existing submodule instead of a copy.
static Stream<Arguments> timestampCorpus() throws IOException {
final List<Arguments> cases = new ArrayList<>();
final byte[] corpus;
try (InputStream in =
ExperimentationOptOutTest.class.getResourceAsStream(
"/experimentation-opt-out-timestamps.jsonl")) {
corpus = readAll(in);
}
for (final String line : new String(corpus, UTF_8).split("\n")) {
if (!line.trim().isEmpty()) {
final Map<String, Object> row = JsonReading.readObject(line);
cases.add(Arguments.of(row.get("id"), row.get("input"), row.get("expected")));
}
}
return cases.stream();
}
private static byte[] readAll(final InputStream in) throws IOException {
final ByteArrayOutputStream out = new ByteArrayOutputStream();
final byte[] buffer = new byte[8192];
for (int n; (n = in.read(buffer)) != -1; ) {
out.write(buffer, 0, n);
}
return out.toByteArray();
}
@ParameterizedTest(name = "{0}")
@MethodSource("timestampCorpus")
void timestampGrammarMatchesSharedCorpus(
final String id, final String input, final Map<String, Object> expected) {
final MutableContext context = new MutableContext().add(ExperimentationOptOut.ATTRIBUTE, input);
if (Boolean.TRUE.equals(expected.get("accepted"))) {
assertEquals(
Instant.ofEpochSecond(
((Number) expected.get("seconds")).longValue(),
((Number) expected.get("nanos")).longValue()),
ExperimentationOptOut.cutoff(ExperimentationOptOutMode.IMMEDIATE, context));
} else {
assertThrows(
IllegalArgumentException.class,
() -> ExperimentationOptOut.cutoff(ExperimentationOptOutMode.IMMEDIATE, context));
}
}It needs these imports in ExperimentationOptOutTest: com.datadog.openfeature.internal.JsonReading, java.io.ByteArrayOutputStream, java.io.IOException, java.io.InputStream, java.util.ArrayList, java.util.List, java.util.Map, and static java.nio.charset.StandardCharsets.UTF_8. The test exercises the cutoff path. Activation parsing calls the same OptOutTimestamps.parse, so the grammar is shared, but an activation-side test would still pin the wiring.
5. Verification
- With all the fixes from my comments applied,
./gradlew :dd-openfeature:testpasses all 1325 tests. - Swapping the parser back to
ISO_OFFSET_DATE_TIMEturns the 8 corpus rows above red. - The cutoff parse alone drops from about 400 ns and 1,512 B to about 37 ns and 24 B per call (JDK 21 microbenchmark). The PR parses once per evaluation for an opted-out subject.
| return ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY; | ||
| default: | ||
| throw new IllegalArgumentException( | ||
| "DD_FEATURE_FLAGS_EXPERIMENTATION_OPT_OUT_MODE must be disabled, immediate, or future_experiments_only"); |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] On an unsupported mode, log a warning and fall back to disabled instead of throwing from the Provider constructor.
The mode is read through the agent's config layer, so it can come from the environment, a system property, or fleet and local stable config. A typo there throws IllegalArgumentException from new Provider(). It throws even with DD_FEATURE_FLAGS_ENABLED=false. In usual startup code, such as the smoke app's Spring @Bean, the application then fails to start. Other bad settings in this module log a warning instead: an invalid boolean reads as false, an invalid positive integer falls back to the next source or the default, and an unsupported configuration source disables Feature Flags. Falling back to disabled removes this constructor exception. The trade-off is that, until the typo is fixed, no subject is opted out; the warning names the bad value.
This departs from draft decision D015 ("enabled providers reject unsupported modes") in DataDog/ffe-system-test-data#55, so the contract needs the same change.
Evidence — executed against this PR
| Configuration | Result |
|---|---|
DD_FEATURE_FLAGS_ENABLED=false, mode immedaite |
IllegalArgumentException from new Provider() |
mode future-experiments-only (hyphens) |
IllegalArgumentException |
OpenFeatureAPI.setProviderAndWait(...) never receives the provider, so nothing in the OpenFeature SDK catches the exception. In the smoke app's Spring @Bean, bean creation fails.
Suggested fix — warn and disable; compiles on its own against this PR
--- a/dd-openfeature/src/main/java/com/datadog/openfeature/internal/evaluation/ExperimentationOptOut.java
+++ b/dd-openfeature/src/main/java/com/datadog/openfeature/internal/evaluation/ExperimentationOptOut.java
@@ -11,9 +11,12 @@
import java.util.List;
import java.util.Locale;
import java.util.Objects;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/** Validates the subject's opt-out timestamp before allocation selection. */
public final class ExperimentationOptOut {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ExperimentationOptOut.class);
public static final String ATTRIBUTE = "dd_experimentation_opt_out_at";
public static final String INVALID_TIMESTAMP =
ATTRIBUTE
@@ -39,8 +42,12 @@
case "future_experiments_only":
return ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY;
default:
- throw new IllegalArgumentException(
- "DD_FEATURE_FLAGS_EXPERIMENTATION_OPT_OUT_MODE must be disabled, immediate, or future_experiments_only");
+ // A typo must not stop the application. Without a valid mode, no subject is opted out.
+ LOGGER.warn(
+ "Unsupported experimentation opt-out mode '{}'; expected disabled, immediate, or"
+ + " future_experiments_only. Experimentation opt-out is disabled.",
+ configured);
+ return ExperimentationOptOutMode.DISABLED;
}
}
providerConfigurationPrecedence["unsupported"] then expects DISABLED instead of a throw. A test that pins the disabled case, from the test class in my comment on the exclusion loop:
// An unsupported mode does not stop construction when Feature Flags is disabled.
@Test
void disabledFeatureFlagsDoNotValidateMode() {
final String[][] properties = {
{"dd.feature.flags.enabled", "false"},
{"dd.feature.flags.experimentation.opt.out.mode", "unsupported"}
};
final String[] previous = new String[properties.length];
for (int i = 0; i < properties.length; i++) {
previous[i] = System.setProperty(properties[i][0], properties[i][1]);
}
try {
assertDoesNotThrow(() -> new Provider().shutdown());
} finally {
for (int i = 0; i < properties.length; i++) {
if (previous[i] == null) {
System.clearProperty(properties[i][0]);
} else {
System.setProperty(properties[i][0], previous[i]);
}
}
}
}With all the fixes from my comments applied, the 1325 module tests pass. Restoring the throw turns this test and providerConfigurationPrecedence["unsupported"] red.
|
|
||
| if (experimentationCutoff != null | ||
| && experimentationOptOutMode == ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY | ||
| && ExperimentationOptOut.hasInvalidActivation(flag.allocations)) { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Move this activation check below the !flag.enabled return, so a disabled flag reports DISABLED, as it does in libdatadog.
Today a disabled flag with a malformed randomizationActivatedAt returns ERROR/PARSE_ERROR for an opted-out subject in future mode. libdatadog rejects a disabled flag at compile time (FlagDisabled), before it checks activation, so Ruby reports DISABLED. The value is the default either way. The reason and error code differ between the two SDKs, and so do the evaluation metrics and flag-evaluation events where those are enabled. This only affects opted-out subjects.
No test pins this order: moving the block below the disabled return leaves all 1264 module tests green.
Suggested fix and test — the test is red on this PR and green with the fix
--- a/dd-openfeature/src/main/java/com/datadog/openfeature/internal/evaluation/FlagEvaluator.java
+++ b/dd-openfeature/src/main/java/com/datadog/openfeature/internal/evaluation/FlagEvaluator.java
@@ -129,6 +129,14 @@ public final class FlagEvaluator {
return error(defaultValue, ErrorCode.FLAG_NOT_FOUND, null, observeFullEvaluationData);
}
+ if (!flag.enabled) {
+ return ProviderEvaluation.<T>builder()
+ .value(defaultValue)
+ .reason(Reason.DISABLED.name())
+ .flagMetadata(consentMetadata(observeFullEvaluationData))
+ .build();
+ }
+
if (experimentationCutoff != null
&& experimentationOptOutMode == ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY
&& ExperimentationOptOut.hasInvalidActivation(flag.allocations)) {
@@ -139,14 +147,6 @@ public final class FlagEvaluator {
observeFullEvaluationData);
}
- if (!flag.enabled) {
- return ProviderEvaluation.<T>builder()
- .value(defaultValue)
- .reason(Reason.DISABLED.name())
- .flagMetadata(consentMetadata(observeFullEvaluationData))
- .build();
- }
-
if (flag.allocations == null) {
return error(
defaultValue, // A disabled flag reports DISABLED before activation timestamps are checked, as in libdatadog.
@Test
void disabledFlagReportsDisabledBeforeInvalidActivation() throws Exception {
final ProviderEvaluation<String> result =
evaluate(
configuration(false, "\"bad\""),
ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY,
new MutableContext("subject").add(ATTRIBUTE, CUTOFF));
assertNull(result.getErrorCode());
assertEquals(Reason.DISABLED.name(), result.getReason());
assertEquals("default", result.getValue());
}This test is also part of the test class in my comment on the exclusion loop, which supplies the configuration and evaluate helpers. Take it from either place.
| for (final Allocation allocation : flag.allocations) { | ||
| if (experimentationCutoff != null | ||
| && ExperimentationOptOut.excludes( | ||
| experimentationOptOutMode, experimentationCutoff, allocation)) { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Add a test that an excluded allocation's rules are never evaluated.
The order here is correct: exclusion comes before isAllocationActive, rules, and shards. But no test pins the order relative to rules. Moving the exclusion below evaluateRules leaves all 1264 module tests green. If that order regressed, an excluded allocation with a numeric rule on a non-numeric attribute would turn an opted-out subject's ordinary result into TYPE_MISMATCH.
Suggested test class — excludedAllocationRulesAreNotEvaluated is red on that reordering and green on this PR
The whole file, including the tests from my other comments. It compiles in dd-openfeature/src/test/java/com/datadog/openfeature/.
package com.datadog.openfeature;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import com.datadog.openfeature.internal.bridge.v1.AgentBridge;
import com.datadog.openfeature.internal.config.TestSettings;
import com.datadog.openfeature.internal.ufc.UniversalFlagConfigParser;
import dev.openfeature.sdk.ErrorCode;
import dev.openfeature.sdk.MutableContext;
import dev.openfeature.sdk.MutableStructure;
import dev.openfeature.sdk.ProviderEvaluation;
import dev.openfeature.sdk.Reason;
import dev.openfeature.sdk.Value;
import java.time.Instant;
import java.util.Arrays;
import java.util.Collections;
import java.util.stream.Stream;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.MethodSource;
/** Pins opt-out behavior the policy corpus does not reach. */
class ExperimentationOptOutGapTest {
private static final String ATTRIBUTE = "dd_experimentation_opt_out_at";
private static final String CUTOFF = "2026-10-19T12:00:00Z";
/** The experiment allocation is sharded and has a numeric rule; the ordinary one is neither. */
private static String configuration(final boolean enabled, final String activation) {
final String experiment =
"{\"key\":\"experiment\",\"doLog\":true"
+ (activation == null ? "" : ",\"randomizationActivatedAt\":" + activation)
+ ",\"rules\":[{\"conditions\":[{\"attribute\":\"age\",\"operator\":\"GT\",\"value\":18}]}]"
+ ",\"splits\":[{\"variationKey\":\"experiment\",\"shards\":[{\"salt\":\"s\",\"totalShards\":10000,"
+ "\"ranges\":[{\"start\":0,\"end\":10000}]}]}]}";
final String ordinary =
"{\"key\":\"ordinary\",\"doLog\":false,\"splits\":[{\"variationKey\":\"ordinary\",\"shards\":[]}]}";
return "{\"flags\":{\"target\":{\"key\":\"target\",\"enabled\":"
+ enabled
+ ",\"variationType\":\"STRING\",\"variations\":{"
+ "\"experiment\":{\"key\":\"experiment\",\"value\":\"experiment\"},"
+ "\"ordinary\":{\"key\":\"ordinary\",\"value\":\"ordinary\"}},"
+ "\"allocations\":["
+ experiment
+ ","
+ ordinary
+ "]}}}";
}
private static ProviderEvaluation<String> evaluate(
final String json, final ExperimentationOptOutMode mode, final MutableContext context)
throws Exception {
final DDEvaluator evaluator =
new DDEvaluator(() -> {}, AgentBridge.NONE, TestSettings.of(), mode);
evaluator.accept(UniversalFlagConfigParser.parse(json.getBytes(UTF_8)));
return evaluator.evaluate(String.class, "target", "default", context);
}
// Exclusion happens before randomization: a skipped allocation needs no targeting key.
@ParameterizedTest
@EnumSource(
value = ExperimentationOptOutMode.class,
names = {"IMMEDIATE", "FUTURE_EXPERIMENTS_ONLY"})
void excludedAllocationSkipsShardingWithoutTargetingKey(final ExperimentationOptOutMode mode)
throws Exception {
final MutableContext context = new MutableContext().add(ATTRIBUTE, CUTOFF).add("age", 30);
final ProviderEvaluation<String> result = evaluate(configuration(true, null), mode, context);
assertNull(result.getErrorCode());
assertEquals("ordinary", result.getValue());
// Control: without opt-out the experiment's shards need the targeting key.
final ProviderEvaluation<String> control =
evaluate(configuration(true, null), ExperimentationOptOutMode.DISABLED, context);
assertEquals(ErrorCode.TARGETING_KEY_MISSING, control.getErrorCode());
}
// Exclusion happens before targeting: a skipped allocation's rules are never evaluated.
@ParameterizedTest
@EnumSource(
value = ExperimentationOptOutMode.class,
names = {"IMMEDIATE", "FUTURE_EXPERIMENTS_ONLY"})
void excludedAllocationRulesAreNotEvaluated(final ExperimentationOptOutMode mode)
throws Exception {
final MutableContext context =
new MutableContext("subject").add(ATTRIBUTE, CUTOFF).add("age", "not-a-number");
final ProviderEvaluation<String> result = evaluate(configuration(true, null), mode, context);
assertNull(result.getErrorCode());
assertEquals("ordinary", result.getValue());
final ProviderEvaluation<String> control =
evaluate(configuration(true, null), ExperimentationOptOutMode.DISABLED, context);
assertEquals(ErrorCode.TYPE_MISMATCH, control.getErrorCode());
}
// A disabled flag reports DISABLED before activation timestamps are checked, as in libdatadog.
@Test
void disabledFlagReportsDisabledBeforeInvalidActivation() throws Exception {
final ProviderEvaluation<String> result =
evaluate(
configuration(false, "\"bad\""),
ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY,
new MutableContext("subject").add(ATTRIBUTE, CUTOFF));
assertNull(result.getErrorCode());
assertEquals(Reason.DISABLED.name(), result.getReason());
assertEquals("default", result.getValue());
}
static Stream<Arguments> nonStringAttributes() {
return Stream.of(
Arguments.of("Instant", new Value(Instant.parse(CUTOFF))),
Arguments.of("double", new Value(1.5)),
Arguments.of("structure", new Value(new MutableStructure().add("at", CUTOFF))),
Arguments.of("list", new Value(Collections.singletonList(new Value(CUTOFF)))),
Arguments.of("empty list", new Value(Arrays.asList())),
Arguments.of("null value", new Value()));
}
// Any present non-string value is INVALID_CONTEXT, including a java.time.Instant.
@ParameterizedTest(name = "{0}")
@MethodSource("nonStringAttributes")
void nonStringAttributeIsInvalidContextOnlyWhenEnabled(final String name, final Value value)
throws Exception {
for (final ExperimentationOptOutMode mode : ExperimentationOptOutMode.values()) {
final MutableContext context =
new MutableContext("subject", Collections.singletonMap(ATTRIBUTE, value));
final ProviderEvaluation<String> result =
evaluate(configuration(true, "\"2026-01-01T00:00:00Z\""), mode, context);
if (mode == ExperimentationOptOutMode.IMMEDIATE
|| mode == ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY) {
assertEquals(ErrorCode.INVALID_CONTEXT, result.getErrorCode(), name + " " + mode);
assertEquals("default", result.getValue(), name + " " + mode);
} else {
// The age rule does not match, so the subject falls through to ordinary.
assertNull(result.getErrorCode(), name + " " + mode);
assertEquals("ordinary", result.getValue(), name + " " + mode);
}
}
}
// An unsupported mode does not stop construction when Feature Flags is disabled.
@Test
void disabledFeatureFlagsDoNotValidateMode() {
final String[][] properties = {
{"dd.feature.flags.enabled", "false"},
{"dd.feature.flags.experimentation.opt.out.mode", "unsupported"}
};
final String[] previous = new String[properties.length];
for (int i = 0; i < properties.length; i++) {
previous[i] = System.setProperty(properties[i][0], properties[i][1]);
}
try {
assertDoesNotThrow(() -> new Provider().shutdown());
} finally {
for (int i = 0; i < properties.length; i++) {
if (previous[i] == null) {
System.clearProperty(properties[i][0]);
} else {
System.setProperty(properties[i][0], previous[i]);
}
}
}
}
}excludedAllocationSkipsShardingWithoutTargetingKey is a direct check only: moving the targeting-key check before the exclusion already turns the shared special-characters fixture cases red. disabledFlagReportsDisabledBeforeInvalidActivation and disabledFeatureFlagsDoNotValidateMode are red on this PR until the fixes in my other comments are applied.
| /** Controls per-subject exclusion from experimentation. */ | ||
| public enum ExperimentationOptOutMode { | ||
| /** Reads DD_FEATURE_FLAGS_EXPERIMENTATION_OPT_OUT_MODE, which defaults to disabled. */ | ||
| DEFAULT, |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Consider removing the public DEFAULT member and modeling "unset" as a null option, as enableEvaluationTelemetryOptOutParameter already does.
DEFAULT is an instruction to read configuration, not a mode. Provider resolves it first, but the internal FlagEvaluator and DDEvaluator constructors still accept it and treat it as disabled. Customers can also write experimentationOptOutMode(DEFAULT). Once released, removing the member breaks source and binary compatibility. The sibling option uses Boolean null for "unset", and Ruby uses nil.
Sketch — not compiled; it also touches two tests and the README
// ExperimentationOptOutMode: delete DEFAULT.
// Provider.Options
private ExperimentationOptOutMode experimentationOptOutMode; // null: read configuration
/**
* Sets per-subject experimentation opt-out. Overrides
* {@code DD_FEATURE_FLAGS_EXPERIMENTATION_OPT_OUT_MODE}. Without an explicit option, the provider
* reads configuration at construction. Disabled by default.
*/
public Options experimentationOptOutMode(final ExperimentationOptOutMode mode) {
this.experimentationOptOutMode = requireNonNull(mode, "mode");
return this;
}
// ExperimentationOptOut.resolve: drop requireNonNull(override, …), then
if (override != null) {
return override;
}requireNonNull needs import static java.util.Objects.requireNonNull;. Tests that use DEFAULT today: ExperimentationOptOutProviderTest (to reset) and ExperimentationOptOutTest (resolve(DEFAULT, …)). README line 38 also names it.
|
|
||
| Omit the attribute for subjects who can participate. | ||
| Invalid, null, empty, or non-string values return the application's default with `INVALID_CONTEXT` and a diagnostic. | ||
| Timestamp validation uses the same parser as allocation `startAt` and `endAt`. |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Clarify the README in four places: the timestamp grammar, the scope of INVALID_CONTEXT, unsupported modes, and the backend dependencies.
- "a valid RFC3339 timestamp" and "Timestamp validation uses the same parser as allocation
startAtandendAt" describeISO_OFFSET_DATE_TIME, which is not the shared grammar (see my parser comment). State the grammar instead of naming the parser. - "Invalid, null, empty, or non-string values return the application's default with
INVALID_CONTEXT" is true. Once configuration has loaded, it applies to every flag evaluated with that context, including flags without experiments. Say so explicitly. - "Unsupported values throw
IllegalArgumentException" changes if the warn-and-disable suggestion is adopted. - Line 65 covers part of the backend dependency for future mode. Draft decisions D012 and D018 in Add a Lean proof prototype and shared SDK decision history ffe-system-test-data#55 record both dependencies:
- If a completed winner rollout still carries
doLog=true, it is skipped for opted-out subjects: always in immediate mode, and in future mode unless its activation is before the cutoff. Those subjects get the next matching allocation or the default. - If
randomizationActivatedAtis absent, future mode skips everydoLog=trueallocation for an opted-out subject, the same as immediate mode.
- If a completed winner rollout still carries
Suggested text — assumes the parser and warn-and-disable suggestions are adopted
`dd_experimentation_opt_out_at` must be a `String`: a four-digit year, a full date and time with
seconds (`00`–`59`), `T` or `t`, optional fractional seconds (digits after the ninth are ignored),
and `Z`, `z`, or a `±HH:MM` offset. The instant must fall in UTC years 0000–9999.
For example, `2026-10-19T12:00:00Z`. `java.time.Instant` values are rejected.
In an enabled mode, once configuration has loaded, an invalid value makes every flag evaluation
for that context return the application's default with `INVALID_CONTEXT`. This includes flags without experiments and flags
that do not exist.
An unsupported mode logs a warning and disables experimentation opt-out.
Until the configuration service marks completed winner rollouts `doLog=false`, opted-out subjects
can skip those rollouts and receive the next matching allocation or the default. Future mode also
needs `randomizationActivatedAt`. Until the configuration service supplies it, future mode skips
every `doLog=true` allocation for an opted-out subject, the same as immediate mode.|
|
||
| /** Returns the configured experimentation opt-out mode, disabled by default. */ | ||
| public String experimentationOptOutMode() { | ||
| final String mode = get("feature.flags.experimentation.opt.out.mode"); |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] A few small cleanups, take or leave.
- Setting key. This is the only inline key literal in
Settings. Every other key is a constant (EVALUATION_TELEMETRY_PER_SUBJECT_OPT_OUT_ENABLEDand others). Addstatic final String EXPERIMENTATION_OPT_OUT_MODE = "feature.flags.experimentation.opt.out.mode";and reuse it inSettingsTest. requireNonNull.Provider.Optionswritesjava.util.Objects.requireNonNullinline. The module imports it statically elsewhere (ExposureHook).- Activation validity.
AllocationcarriesinvalidRandomizationActivationper allocation and has three constructors, andhasInvalidActivationwalks the allocations (up to all of them) on each opted-out future-mode evaluation. The only consumer asks a flag-level question. Compute it once in the parser and store it onFlag. Then the 8-argument constructor and the per-allocation boolean go away. The scan costs about 6–10 ns at 20 allocations, so this is about shape, not speed. - Comments that could be more precise:
Allocation: "Invalid activation is retained for future-mode evaluation errors." Only a boolean is kept, not the value. Suggest: "True whenrandomizationActivatedAtwas present but unparseable. Future mode with a valid subject cutoff then fails the flag withPARSE_ERROR."Allocation: "null means unknown". It is also null when the value is invalid.ExperimentationOptOutMode.FUTURE_EXPERIMENTS_ONLY: "Unknown activation is skipped." That holds for missing or null. A malformed value fails the whole flag withPARSE_ERRORwhen the subject has a valid cutoff.ExperimentationOptOut: "Validates the subject's opt-out timestamp before allocation selection." The class also resolves the mode and decides exclusion.
| @@ -0,0 +1,5 @@ | |||
| # Experimentation opt-out test cases | |||
|
|
|||
| `cases.json` covers 90 combinations of opt-out mode, subject timestamp, allocation activation, | |||
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] Read these 90 cases from the shared corpus instead of a separate copy, or at least record where they come from.
cases.json matches the 90 "kind": "policy" cases of contracts/experimentation-opt-out/corpus/browser-proposal.jsonl in ffe-system-test-data on args and expected. Only the id prefix, decision_ids, and kind were dropped. Ruby carries a copy of the same lines, and libdatadog carries another. No check compares this copy with its source. The test checks the count, unique ids, and each case's result, so an upstream change would not be noticed. It also lives under dd-smoke-tests, but only the dd-openfeature unit test reads it.
Suggested approach
- Once Add a Lean proof prototype and shared SDK decision history ffe-system-test-data#55 merges, bump the existing
ffe-system-test-datasubmodule. Readbrowser-proposal.jsonlfrom it, keep the"kind": "policy"lines, and deletecases.json. The contract corpus also carries the timestamp rows from my parser comment. - Until then, add to this README the source path, the ffe-system-test-data commit, and the extraction rule. Ruby's fixture README already records a source SHA-256.
| if (cutoff == null) { | ||
| return false; | ||
| } | ||
| if (!Boolean.TRUE.equals(allocation.doLog)) { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
Heads up: SDKs disagree on what an omitted doLog means. I suggest one shared rule for every SDK, client and server: omitted (or null) means false, for both exposure logging and opt-out.
Java already does this, here and in exposure logging (Boolean.TRUE.equals). libdatadog and Go treat an omitted doLog as true for both. The Node core and the open .NET opt-out PR log no exposure for it, but exclude it as an experiment. Draft decision D010 in DataDog/ffe-system-test-data#55 calls omission invalid input but sets no default, so the split stays open.
If exposure-logging allocations are the minority, a false default also lets the configuration omit doLog on ordinary allocations and save bytes.
What each SDK does today — explicit true/false agree everywhere
| SDK | Omitted: exposure? | Omitted: opt-out experiment? | Null |
|---|---|---|---|
| libdatadog (Ruby, Python, PHP) | yes (default_do_log → true) |
yes | that flag fails to parse |
| Go | yes (doLog := true unless set) |
yes | same as omitted |
| Java, this PR | no | no | same as omitted |
| .NET, DataDog/dd-trace-dotnet#9470 | no | yes (DoLog != false) |
same as omitted |
| Node core, openfeature-js-client | no (!!allocation.doLog) |
yes (doLog !== false) |
same as omitted |
| browser, iOS, Android | server-computed; a missing doLog is rejected as invalid |
server-side | rejected |
No allocation in the ffe-system-test-data fixtures omits doLog or sets it to null today, so a shared default changes no fixture result. It would change libdatadog and Go behavior for any configuration that omits doLog, and libdatadog's for a null doLog (from a parse failure to ordinary evaluation), so the configuration producers need to confirm they always send it. The .NET and Node rows describe an open PR and current main.
Suggested contract rule
- An omitted or null
doLogmeansfalse: no exposure, and not an experiment allocation for opt-out. - Add policy rows with omitted and null
doLogto the shared corpus, for both modes. - libdatadog changes
default_do_logtofalseand accepts null. Go changes its nil default. .NET and the Node core change their opt-out checks to== true. - Java needs no change.
What Does This Do
Subjects can opt out of experimentation while continuing to receive ordinary feature allocations. Configure
DD_FEATURE_FLAGS_EXPERIMENTATION_OPT_OUT_MODEbefore constructing the provider and supplydd_experimentation_opt_out_atin the evaluation context.disabled(default)immediatefuture_experiments_onlyExplicit provider options override configuration. The deprecated provider alias uses the same evaluator and supports the same options.
Motivation
Customers need per-subject experimentation controls without losing ordinary feature delivery or independently configured evaluation telemetry.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: FFLSDK-258
FFE reviewer selection
Reviewers: @greghuels, @sameerank
Why: Privacy opt-out and event logging rules — The PR adds a per-subject (per-evaluation-context) experimentation opt-out to the Java OpenFeature provider. Global reviewers cover a shortage in the matched pod.
🤖 Automated selection.
Selection details
Initiative: project / ufc-compliance
Why this match: The PR adds a per-subject (per-evaluation-context) experimentation opt-out to the Java OpenFeature provider. When the context contains dd_experimentation_opt_out_at and the mode is immediate or future_experiments_only, the evaluator skips allocations that have doLog=true, so no experiment exposure events are logged for that subject. Ordinary feature delivery keeps working. This matches the project's purpose: per-context suppression of exposure event egress to honor a user opt-out. The new configuration setting, the provider options, the allocation activation-time parsing and the tests all support that opt-out behavior. It is not layers, holdouts, bandits or EVP evaluation-telemetry work.
Why these owners: The matched initiative has fewer than two approved, non-author candidates. The approved global pool supplies the remaining review owners; proposed memberships were not activated.
The PR author was excluded. Existing reviewers and teams were preserved.
Debug or fix a problem
Workflow run · Bits Code session
dry_run=false. Unchanged completed runs are skipped. If the receipt saysneeds_reconciliationor a write outcome is uncertain, ask the workflow owner to reconcile GitHub and the receipt before retrying; do not delete the lock blindly.Evidence and run details
✅ Complete — reviewer coverage verified. The workflow checked pending requests or submitted reviews on the evaluated head.
Generated asynchronously by the FFE reviewer-routing workflow.
Changed paths:
dd-openfeature/src/main/java/com/datadog/openfeature/internal/evaluation/ExperimentationOptOut.java,dd-openfeature/src/main/java/com/datadog/openfeature/internal/evaluation/FlagEvaluator.java,dd-openfeature/src/main/java/com/datadog/openfeature/ExperimentationOptOutMode.java,dd-openfeature/src/main/java/com/datadog/openfeature/Provider.java,dd-openfeature/src/main/java/com/datadog/openfeature/internal/config/Settings.java,dd-openfeature/src/main/java/com/datadog/openfeature/internal/ufc/Allocation.java,dd-openfeature/src/main/java/com/datadog/openfeature/internal/ufc/UniversalFlagConfigParser.java,dd-openfeature/README.md,metadata/supported-configurations.jsonEvaluated head:
55ba0bdad77e02a3398975429a66f80aac1a05a3Registry revision:
revision:1946aa2a-530e-487a-84a7-83aff44a1089Pinned skill source:
d214cfc65ae154b97d39af5cf32caf48aaec9bf0Instructions: workflow-injected snapshot (no plugin loaded)
Instruction snapshot SHA-256:
f23b2a88c13c0da90e2dd58246a14f257af91e2195c555039bd555b52e6f3516